Skip to content
This repository has been archived by the owner on Oct 28, 2022. It is now read-only.

proposed sequence annotation API #246

Merged
merged 1 commit into from
Mar 30, 2015
Merged

Conversation

diekhans
Copy link
Contributor

@diekhans diekhans commented Mar 4, 2015

Pull request for sequence annotation from topic branch.

This impacts several other task teams, so please examine and vote.

@mbaudis
Copy link
Member

mbaudis commented Mar 4, 2015

Courageous effort, and much wished for progress! However, at the moment, I'm not sure about all points. Things immediately coming to mind:

  • I support moving the OntologyTerm from metadata to common. However, its structure is being much discussed and probably will change.
  • Region resides in common, but doesn't have an associated reference. While I would understand this implementation wise (you may not want to name your reference for each SNP, CNV etc.) - shouldn't it be somehow guaranteed in the schema definition? Or may I have missed a strict inheritance path?

So - enlightenment I hope for!

@diekhans
Copy link
Contributor Author

diekhans commented Mar 4, 2015

I believe that common needs to be removed all together and
specific modules create for ontologies, coordinates,
cigar. Modules like common' andutils' do nothing to aid
organization and understand. They tend to grow to make things
even more confusing.

Region contains Position which contains the references.

We must get things implemented for them to be real.

Michael Baudis notifications@github.com writes:

Courageous effort, and much wished for progress! However, at the moment, I'm
not sure about all points. Things immediately coming to mind:

• I support moving the OntologyTerm from metadata to common. However, its
structure is being much discussed and probably will change.
• Region resides in common, but doesn't have an associated reference. While I
would understand this implementation wise (you may not want to name your
reference for each SNP, CNV etc.) - shouldn't it be somehow guaranteed in
the schema definition? Or may I have missed a strict inheritance path?

So - enlightenment I hope for!


Reply to this email directly or view it on GitHub.*

@mbaudis
Copy link
Member

mbaudis commented Mar 4, 2015

+1

Region contains Position which contains the references.

O.k., I didn't follow this (would also be a nice place for a comment there, for Avro blind ppl. like me ...).

As for the detangling, at least I'm glad if the ontology format specification gets out of metadata...

@buske
Copy link
Member

buske commented Mar 13, 2015

The Matchmaker group is also interested in implementing paging, so we'll likely adopt whatever naming you use here.

@kellrott
Copy link
Member

+1
The Genotype2Phenotype API is already connecting to elements created in the branch. It will be nice to have these concepts join the master branch.

/**
Type defining a collection of attributes associated with various protocol
records. Each attribute is a name that maps to an array of one or more
values. To work around Avro restrictions, three fields are define, one for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should say "defined", not "define".

@adamnovak
Copy link
Contributor

I'm +1 on what's here, but we appear to be missing the wiggle methods (as is noted in the code). Those should either be added on to this PR, or come soon after.

@nlwashington
Copy link
Contributor

+1. We intend to use it for G2P. I would recommend, however, that the record name of Feature change to GenomicFeature. This is to make it more descriptive, and avoid conflict with the matchmaker API (that also has Feature record for phenotype -- though that should also probably be updated too).

@buske
Copy link
Member

buske commented Mar 25, 2015

@nlwashington Happy to update the Matchmaker term to something more explicit.

@pgrosu
Copy link
Contributor

pgrosu commented Mar 25, 2015

General confirmation question: Matchmaker API is still part of schemas, or does it have another github location for discussions?

Thanks,
Paul

@nlwashington
Copy link
Contributor

@buske that sounds great. perhaps PhenotypicFeature?

@buske
Copy link
Member

buske commented Mar 25, 2015

@pgrosu We have another github repo for internal discussions, but I've been porting our API to Avro within schemas (see #258) and have occasionally been posting questions to the schemas issue tracker.

@nlwashington Sounds good. I'll change this within our current outstanding PR #258, since I don't think we're set on any of the object names (we don't use them in our JSON API, so they're a necessary result of porting to Avro).

@buske
Copy link
Member

buske commented Mar 25, 2015

@nlwashington Realized we also have a GenomicFeature class, so that would be another name collision. I'm not sure what the right way to proceed is. I'm happy to prefix all our classes with MME, or is the different namespace sufficient?: @namespace("org.ga4gh.match")

@diekhans
Copy link
Contributor Author

A one point before the sequence annotation records had a prefix
(SeqAnn*), but that was removed as not-GA4GH like. While namespaces
can prevent collisions in the software, it can still be confusing.

We need to do a global review of the names and other patterns
in the APIs, but first we need to get all the current work in so
we have something to review!

Nicole Washington notifications@github.com writes:

+1. We intend to use it for G2P. I would recommend, however, that the record
name of Feature change to GenomicFeature. This is to make it more descriptive,
and avoid conflict with the matchmaker API (that also has Feature record for
phenotype -- though that should also probably be updated too).


Reply to this email directly or view it on GitHub.*

@pgrosu
Copy link
Contributor

pgrosu commented Mar 25, 2015

Thank you @buske for bringing me up to speed on this and for the link.

Paul

@benedictpaten
Copy link
Contributor

I like 99% of what's there. I'm nearly 1+, however, I'm a bit unsure about the QueryRange object. Would it not be better to just use a path?

I also agree with @adamnovak that having the wiggle methods in would be nice.

@diekhans
Copy link
Contributor Author

Sounds good. Edit to you liking..

... Sent from my computer phone

-----Original Message-----
From: Benedict Paten notifications@github.com
To: ga4gh/schemas schemas@noreply.github.com
Cc: Mark Diekhans markd@soe.ucsc.edu
Sent: Wed, 25 Mar 2015 1:52 PM
Subject: Re: [schemas] proposed sequence annotation API (#246)

I like 99% of what's there. I'm nearly 1+, however, I'm a bit unsure about the QueryRange object. Would it not be better to just use a path?

I also agree with @adamnovak that having the wiggle methods in would be nice.


Reply to this email directly or view it on GitHub:
#246 (comment)

@benedictpaten
Copy link
Contributor

Not trying to weasel out of work, but I'd be happy if you could just drop
the QueryRange object and change lines:

/**+ If specified, this query matches only annotations which overlap this
range.+ */+ union { null, QueryRange } range = null;

to

/**+ If specified, this query matches only annotations which overlap this
path.+ */+ union { null, Path } path = null;
I can do this - but I'd need to create a new pull request - much better if
you keep your momentum and +1s!

Note for others reading this - Mark uses path to specify wiggles/features,
so this is consistent.

On Wed, Mar 25, 2015 at 3:52 PM, Mark Diekhans notifications@github.com
wrote:

Sounds good. Edit to you liking..

... Sent from my computer phone

-----Original Message-----
From: Benedict Paten notifications@github.com
To: ga4gh/schemas schemas@noreply.github.com
Cc: Mark Diekhans markd@soe.ucsc.edu
Sent: Wed, 25 Mar 2015 1:52 PM
Subject: Re: [schemas] proposed sequence annotation API (#246)

I like 99% of what's there. I'm nearly 1+, however, I'm a bit unsure about
the QueryRange object. Would it not be better to just use a path?

I also agree with @adamnovak that having the wiggle methods in would be
nice.


Reply to this email directly or view it on GitHub:
#246 (comment)


Reply to this email directly or view it on GitHub
#246 (comment).

@diekhans
Copy link
Contributor Author

Updated to use Path (sorry, delayed by having a lame Internet connection).

We need to get all the methods in there, but I think get the
data structures in the master so others can build on it is
really important.

I do expect this to change as the other groups link with it.

Benedict Paten notifications@github.com writes:

I like 99% of what's there. I'm nearly 1+, however, I'm a bit unsure about the
QueryRange object. Would it not be better to just use a path?

I also agree with @adamnovak that having the wiggle methods in would be nice.


Reply to this email directly or view it on GitHub.*

@benedictpaten
Copy link
Contributor

1+, the other methods can be added later.

Benedict
(from phone)
On Mar 29, 2015 11:22 AM, "Mark Diekhans" notifications@github.com wrote:

Updated to use Path (sorry, delayed by having a lame Internet connection).

We need to get all the methods in there, but I think get the
data structures in the master so others can build on it is
really important.

I do expect this to change as the other groups link with it.

Benedict Paten notifications@github.com writes:

I like 99% of what's there. I'm nearly 1+, however, I'm a bit unsure
about the
QueryRange object. Would it not be better to just use a path?

I also agree with @adamnovak that having the wiggle methods in would be
nice.


Reply to this email directly or view it on GitHub.*


Reply to this email directly or view it on GitHub
#246 (comment).

@jeromekelleher
Copy link
Contributor

It looks like there are enough +1s here. Can you rebase off upstream/master please and squash the commits please @diekhans?

@diekhans diekhans force-pushed the sequence-annotation branch from ad49765 to ba0fe97 Compare March 30, 2015 15:02
@jeromekelleher
Copy link
Contributor

Thanks @diekhans --- merging.

@jnguyenx jnguyenx mentioned this pull request Apr 8, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants